Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use timeout_in_minutes for Terraform timeout in AWS CloudFormation #5712

Merged
merged 1 commit into from
Mar 21, 2016
Merged

Conversation

andrewtarry
Copy link
Contributor

We recently encountered a problem with the CloudFormation resource due to the timeout_in_minutes value being increased. If the CloudFormation timeout is increased Terraform will timeout after 30 minutes resulting an error and other resources not being applied but CloudFormation carries on building.

This change leave the default timeout at 30 minutes but if the timeout_in_minutes is greater than 30 the timeout becomes timeout_in_minutes + 5 minutes (just to make sure it will have finished or failed)

@@ -143,7 +149,7 @@ func resourceAwsCloudFormationStackCreate(d *schema.ResourceData, meta interface
wait := resource.StateChangeConf{
Pending: []string{"CREATE_IN_PROGRESS", "ROLLBACK_IN_PROGRESS", "ROLLBACK_COMPLETE"},
Target: []string{"CREATE_COMPLETE"},
Timeout: 30 * time.Minute,
Timeout: time.Duration(int64(timeout)) * time.Minute,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have been cleaner to start with int64 variable instead of casting it later.

Not that I would be worried about minutes going over 32bits, but strictly speaking we might potentially end up comparing int32 with int64 in the condition above.

i.e. this

timeout := 30

can be turned into

retryTimeout := int64(30)

and the line here can be turned into

Timeout:    time.Duration(retryTimeout) * time.Minute,

as hinted above, I'd also suggest renaming the timeout variable to more specific retryTimeout.

@radeksimko
Copy link
Member

Hi @andrewtarry
firstly thanks for discovering this edge case and taking the time to submit the PR! 👍

I left you one comment there regarding data type and variable name.

One last thing, before we merge this (and I'm aware I might be asking for too much - depending on your git skills) - would you mind squashing all 3 commits into a single one using git rebase -i HEAD~3 and git push -f? If you're too worried about this, I can do it for you as part of the merging process, but it would be very helpful if you have done that in your fork, so I can just push the green merge button. 😃

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Mar 19, 2016
updated cloudformation timeout to use timeout_in_minutes if greater than 30 minutes

set the retry timeout as int64 when created
@andrewtarry
Copy link
Contributor Author

Hi @radeksimko

Thanks for your comments.

I have removed the extra casting as you suggesting and rebased the commits. Travis seems to be failing but it looks like it's a issue between Travis and github.
Thanks

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Mar 21, 2016
radeksimko added a commit that referenced this pull request Mar 21, 2016
Use timeout_in_minutes for Terraform timeout in AWS CloudFormation
@radeksimko radeksimko merged commit 5c21422 into hashicorp:master Mar 21, 2016
@radeksimko
Copy link
Member

@andrewtarry FYI for the future, it might make your life easier if you create a PR from a feature/bugfix branch, not from master. Generally force-pushing master (even if it's just fork) is not considered a best practice and it makes it difficult to send+maintain more PRs to the same repo. 😉

@ghost
Copy link

ghost commented Apr 27, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants